-
Notifications
You must be signed in to change notification settings - Fork 108
Improve visualizer usability #1382
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Using reflection gives us the declared fields of a class only in alphabetical order, and some information is unnecessarily inside, so we instead just manually give back a list and the data associated with them through companion methods.
The only important fields of a node is its state, props, and rendering. Its and the parent node's ID's are less significant, and have been moved out of their own detailed cards to avoid cluttering up the info panel
All simple nodes (with no children) are shown at the top and nested nodes (with children) are shown at the bottom. This makes it the nesting more clear.
e73317b
to
5ec8a18
Compare
builder.pushStyle(style) | ||
builder.append(text) | ||
builder.pop() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a helper function like this makes it very easy to understand what these three lines are doing, but having to list out the arguments (line 38 vs 43) ends up taking 5 lines instead of 3. Is this a worth it trade off?
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
bb01fc9
to
cdfbc82
Compare
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
59d285a
to
ff128a8
Compare
ff128a8
to
7b7b41a
Compare
8745914
to
a489f00
Compare
83502fe
to
08f412c
Compare
org.jetbrains.kotlin:kotlin-bom:2.1.21 | ||
org.jetbrains.kotlin:kotlin-stdlib-common:2.1.21 | ||
org.jetbrains.kotlin:kotlin-stdlib-jdk7:2.1.21 | ||
org.jetbrains.kotlin:kotlin-stdlib-jdk8:2.1.21 | ||
org.jetbrains.kotlin:kotlin-stdlib:2.1.21 | ||
org.jetbrains.kotlinx:kotlinx-coroutines-bom:1.7.3 | ||
org.jetbrains.kotlinx:kotlinx-coroutines-core-jvm:1.7.3 | ||
org.jetbrains.kotlinx:kotlinx-coroutines-core:1.7.3 | ||
org.jetbrains.kotlin:kotlin-bom:2.2.0 | ||
org.jetbrains.kotlin:kotlin-stdlib-common:2.2.0 | ||
org.jetbrains.kotlin:kotlin-stdlib-jdk7:2.2.0 | ||
org.jetbrains.kotlin:kotlin-stdlib-jdk8:2.2.0 | ||
org.jetbrains.kotlin:kotlin-stdlib:2.2.0 | ||
org.jetbrains.kotlinx:kotlinx-coroutines-bom:1.9.0 | ||
org.jetbrains.kotlinx:kotlinx-coroutines-core-jvm:1.9.0 | ||
org.jetbrains.kotlinx:kotlinx-coroutines-core:1.9.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can update these dependencies to 2.2.0 until Register is on it. At any rate, it's better to do major dependency updates separately from feature changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I missed some things when reverting the dependencies, fixed.
modifier = modifier.onGloballyPositioned { | ||
appWindowSize = it.size | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you only need the size, use onSizeChanged
. onGloballyPositioned
is very expensive. More here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
var frameIndex by remember { mutableIntStateOf(0) } | ||
val sandboxState = remember { SandboxState() } | ||
val nodeLocations = remember { mutableListOf<SnapshotStateMap<Node, Offset>>() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nodeLocations
is read in composition, so it needs to be snapshot state backed.
val nodeLocations = remember { mutableListOf<SnapshotStateMap<Node, Offset>>() } | |
val nodeLocations = remember { mutableStateListOf<SnapshotStateMap<Node, Offset>>() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, the original goal was that I would only want compositions to track what's happening to each SnapshotStateMap
since that's where all the locations are stored, and I would just index into the list. Would my current code not work for that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because when you clear (line 79) or put a new entry in the list (line 100), line 114 needs to know about it since the presence of that index in the list might change.
It's fine to keep the SnapshotStateMap
as well though so you invalidate the map without invalidating the list. Nested state collections are fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay, that makes a lot of sense. Fixed
) { | ||
if (active) { | ||
// Since we can jump from frame to frame, we fill in the map during each recomposition | ||
if (nodeLocations.getOrNull(frameInd) == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (nodeLocations.getOrNull(frameInd) == null) { | |
if (nodeLocations[frameInd] == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think using square brackets to index could throw an IndexOutOfBoundsException right? Having .getOrNull
was for the purpose when we go from frame 0 immediately to frame 10, so we would need to fill in the gap of frames with empty maps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's easy to miss, might be nice to add a comment about that intention.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
workflow-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/App.kt
Outdated
Show resolved
Hide resolved
) { | ||
val relevantNodes = nodes.filter { it.name.contains(searchText, ignoreCase = true) } | ||
Column { | ||
relevantNodes.take(5).forEach { node -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
...trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/util/SandboxBackground.kt
Show resolved
Hide resolved
...-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/util/parser/DiffUtils.kt
Outdated
Show resolved
Hide resolved
...-trace-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/util/parser/DiffUtils.kt
Outdated
Show resolved
Hide resolved
...race-viewer/src/jvmMain/kotlin/com/squareup/workflow1/traceviewer/util/parser/TraceParser.kt
Outdated
Show resolved
Hide resolved
cfa0bd8
to
4a8d29d
Compare
ea48f89
to
6a056ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Resolve the Compose questions/comments from @zach-klippenstein and let's change the example .gifs to a dummy app , but otherwise LGTM.
NEW(Color(0x804CAF50)), // green | ||
STATE_CHANGED(Color(0xFFE57373)), // red | ||
PROPS_CHANGED(Color(0xFFFF8A65)), // orange | ||
CHILDREN_CHANGED(Color(0x802196F3)), // blue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we just call this "RENDERED" or something like that?
When PTR is on then this will represent "Children Changed", but otherwise, that's not always true as all nodes will be rendered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
@@ -55,6 +58,9 @@ internal fun RightInfoPanel( | |||
IconButton( | |||
onClick = { panelOpen = !panelOpen }, | |||
modifier = Modifier | |||
.size(40.dp) | |||
.clip(CircleShape) | |||
.background(Color.White) | |||
.padding(8.dp) | |||
.size(40.dp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
size specified twice? Does order matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope! Removed one now
text = "${node.current.parent} (ID: ${node.current.parentId})", | ||
style = MaterialTheme.typography.subtitle2, | ||
color = Color.Gray, | ||
modifier = Modifier.padding(top = 8.dp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should extract constants/styles for these padding values etc.
(If that needs to be done separately/later, that's fine too!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would all be the kind of stuff that goes in resources? I may not have time to pull out all the styles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't use Android resources for it, if that's what you mean. Just kotlin constants.
emulatorRegex.find(device)?.value?.let { emulator -> | ||
onDeviceSelect(emulator) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOn't we only want to show this card if we find the emulator? Othrewise its a noop.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by "find" the emulator? I have a early return above if that's what you're asking?
} | ||
) { | ||
val text = if (clicked) { | ||
"Trace saved to Downloads" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be great to show the filepath here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
7520167
to
54e15a3
Compare
var frameIndex by remember { mutableIntStateOf(0) } | ||
val sandboxState = remember { SandboxState() } | ||
val nodeLocations = remember { mutableListOf<SnapshotStateMap<Node, Offset>>() } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, because when you clear (line 79) or put a new entry in the list (line 100), line 114 needs to know about it since the presence of that index in the list might change.
It's fine to keep the SnapshotStateMap
as well though so you invalidate the map without invalidating the list. Nested state collections are fine.
*/ | ||
node.children.forEach { (index, childNode) -> | ||
val prevChildNode = previousNode?.children?.get(index) | ||
nestedChildren.forEach { childNode -> | ||
DrawTree( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when a new frame is selected, but that would mean the composable is thrown out and restarted right?
I don't know, it would be if the composable that renders the frame is keyed on the frame. Either way though, sounds like this is fine.
) | ||
|
||
Box { | ||
Box( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I'm reading it correctly, the structure is:
Box {
Box {
Column
}
Tooltip
}
You only need the outer box to layer the column and the tooltip, the inner box only wraps the column and can be eliminated.
text = "${node.current.parent} (ID: ${node.current.parentId})", | ||
style = MaterialTheme.typography.subtitle2, | ||
color = Color.Gray, | ||
modifier = Modifier.padding(top = 8.dp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't use Android resources for it, if that's what you mean. Just kotlin constants.
4f79fb8
to
74fdadf
Compare
21b7902
to
d3645e1
Compare
Signed-off-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Currently